I'm a bit unclear about this part.

1. since Carbon 3 will throw an exception when createFormFormat, I'm not certain it helps to catch it just so we can throw our own. This could even make it harder to spot the reason for why it failed.
2. the second check also seems obsolete since we either get a CarbonImmutable or an Exception gets thrown

So I think this method could be as simple as just line 19 - if something goes wrong an exception gets thrown as it used to. Is there more context for this additional logic that I'm not aware of?

you're right to question it, it's an overthinking part from me. My logic is createFromFormat() may throw a non-instance (null/false), so an extra instanceof guard prevents type error when returning $carbon.

Ah true, I didn't realize the returned value could also be null

@dati18 I like your defensive programming thinking here.

Your reply explains why the === null check was replaced with an instaceof check; but how come the catching and re-throwing of the same exception?

I agree with @deer-wmde here in thinking that allowing the InvalidFormatException to be thrown from createFromFormat() is enough. I'm thinking about the KISS principle.

Thinking out loud some more, if CarbonImmutable::createFromFormat() only returns Carbon or null then if ($carbon === null) should be equivalent to if (!$carbon instanceof CarbonImmutable).

However, given that my PHP knowledge isn't good enough to be 100% confident that createFromFormat() won't return anything other than Carbon or null without throwing an error, and we haven't currently identified this as part of the codebase that needs optimizing, I'm happy to keep using the more explicit instaceof check. 🙂

I think instanceof is more robust against unexpected values and a safer overall check if behavior changes later. So we keep it?

I also updated the exception messages to be clearer and more precise.

As line 21 is expecting the Exception already, I struggle to see how this check helps us

I just want to lock down the exact error message. a bit redundant, yes... WHat do you think? keep it or remove it?

yes, this thought was connected to the other one. If we keep throwing the Exception ourselves I think it's fine to keep it, might even be worth to set the message to something that explicitly makes it obvious it comes from "us" and not just the library

I also tend to shy away from validating error messages unless it is some sort of product requirement or is somehow crucial to the function of the unit under test. I find that in these situations you often end up creating more work for yourself (updating both class and test) without making the program/test suite any more robust.

Is this actually an Invalid Format Exception? I would assume that if it was one of those then CarbonImmutable::createFromFormat() would throw it. At a glance, this seems more like a RuntimeException to me.

Good catch! I was thinking about how to answer this question. My knowledge is that InvalidFormatException is good for catching a single exception type for all "I can't parse MW timestamp" failures. A different type exception (like RuntimeException or UnexpectedValueException) is better if you want to distinguish what kind of exception was caught.
I think using InvalidFormatException is good enough; both cases check for input-parsing failures.